fix(store): read full link sidecar to avoid wrong-symbol truncation#334
Merged
singaraiona merged 2 commits intoJul 18, 2026
Merged
Conversation
Collaborator
|
Please reject a short read before trimming or interning. |
try_load_link_sidecar read the target table's sym name into a fixed 256-byte buffer (fread of 255 bytes). A name longer than 255 bytes was silently truncated, so ray_sym_intern interned a DIFFERENT symbol and the loaded column linked to the wrong table — silent data corruption on a save/load round-trip. The writer already emits the full, untruncated name. Read the whole sidecar into a buffer sized to the file (capped at 1 MiB to bound a corrupt/oversized file), and reject a short read (fread returning fewer bytes than the file size — an I/O error or a race-truncated sidecar) so a partial name can't be interned as a different symbol either. Add a regression test that links through a 300-byte target name and asserts the loaded link_target matches; it fails without the fix.
belowzeroff
force-pushed
the
fix/col-link-sidecar-truncation
branch
from
July 18, 2026 15:42
6c29a63 to
1c63994
Compare
Contributor
Author
|
Thank you. Added an explicit n == fsz short-read check before trimming/interning; a partial read now frees the buffer and leaves the column unlinked. Amended in 1c63994. |
singaraiona
added a commit
that referenced
this pull request
Jul 23, 2026
* v2.4.0 (#327) * feat(query): support live inserts into parted tables Add immutable live-tail growth with explicit partition keys, shared FILE-domain symbol handling, atomic symbol rebinding, adversarial coverage, documentation, and a runnable rollover example. * fix(core): restore total-core -c semantics * fix(parse) Fix nonstring if not defined * fix(store): surface FlushFileBuffers failure in journal SYNC mode (#335) In RAY_JOURNAL_SYNC mode ray_journal_write_bytes checks fsync's return on POSIX and fails the write with RAY_ERR_IO, but the Windows branch ignored FlushFileBuffers' return. A failed flush there was silently swallowed, so SYNC mode reported success while the data may not have reached disk — dropping the durability guarantee the mode exists to provide. Check FlushFileBuffers (0 = failure) and return RAY_ERR_IO, mirroring the POSIX path. Windows-only branch (not built on the Linux/macOS CI matrix), so it is verified by inspection against the adjacent fsync check; the failure path is not unit-testable, like the existing POSIX one. Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> * fix(hnsw): reject build dims whose vector count overflows size_t (#333) ray_hnsw_build sized the copied vector block as n_nodes * dim * sizeof(float) with no overflow check. Dimensions whose product wraps size_t under-allocate the copy while the memcpy — and every later distance read (vectors + id*dim) — run past the buffer. Guard the product before any allocation, mirroring the per-layer neighbor guard in the loader, and reject overflowing dimensions. This hardens the public C API boundary; the in-tree (hnsw-build ...) path sizes vectors from an in-memory list and cannot reach the overflow, so it is defense-in-depth. Add a regression test driving an overflowing n_nodes/dim pair; with the guard removed it faults under ASan (stack-buffer-overflow at the copy). Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> Co-authored-by: Anton Kundenko <singaraiona@gmail.com> * fix(store): read full link sidecar to avoid wrong-symbol truncation (#334) try_load_link_sidecar read the target table's sym name into a fixed 256-byte buffer (fread of 255 bytes). A name longer than 255 bytes was silently truncated, so ray_sym_intern interned a DIFFERENT symbol and the loaded column linked to the wrong table — silent data corruption on a save/load round-trip. The writer already emits the full, untruncated name. Read the whole sidecar into a buffer sized to the file (capped at 1 MiB to bound a corrupt/oversized file), and reject a short read (fread returning fewer bytes than the file size — an I/O error or a race-truncated sidecar) so a partial name can't be interned as a different symbol either. Add a regression test that links through a 300-byte target name and asserts the loaded link_target matches; it fails without the fix. Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> Co-authored-by: Anton Kundenko <singaraiona@gmail.com> * fix(hnsw): reject index files whose vector count overflows size_t (#332) hnsw_load_impl read n_nodes and dim straight from the file header and sized the vectors allocation as n_nodes * dim * sizeof(float) with no overflow check. A crafted header could make that product wrap size_t, so ray_sys_alloc under-allocated the buffer while the following fread still read the full (large) element count and wrote past the allocation — a heap-overflow write driven by an untrusted index file. Factor the check into ray_hnsw_vec_size_valid(n_nodes, dim) and reject the header before any allocation, mirroring the per-layer neighbor guard. Add a unit test that drives the helper directly (ordinary dims, non-positive dims, an overflowing pair, and the exact size_t boundary). It is tested at the helper rather than through ray_hnsw_load because an overflow-patched header is refused earlier — the huge node-level read fails first — so a full-load test could not distinguish the guard. Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> * fix(docs): remediate F-0001 F-0005 F-0007 Escalate F-0002, F-0003, F-0004, and F-0006 into CF-0001 through CF-0004 after the required corpus census. * fix(docs): remediate CF-0001 * fix(docs): remediate CF-0002 * chore(audit): plan CF-0003 ratification * fix(docs): remediate CF-0003 * feat(docs): redesign website and documentation Rebuild the MkDocs and marketing surfaces around the Rayforce brand, add the live market demo and cloud preview, unify responsive navigation, and eliminate reload layout shifts. * fix(null): avoid f64 null casts to integers (#340) * fix(null): avoid f64 null casts to integers * fix(expr): guard f64 to i64 fallback casts * fix(null): clamp finite f64 narrow casts --------- Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> * fix(expr): avoid null truthiness casts in fallback binary ops (#339) * fix(expr): avoid null truthiness casts in fallback binary ops binary_range's fallback OP_AND/OP_OR kernels cast the widened `double` operand straight to `uint8_t`: uint8_t li = (uint8_t)LV_READ(i); `LV_READ` widens integer operands to `double` and yields NaN for float nulls, so this had two defects: - Wrong answers from 8-bit truncation: `(uint8_t)256.0 == 0`, so `256 and 1b` returned false. - Undefined behavior: casting NaN (NULL_F64) or a widened NULL_I64 (-9.2e18) to `uint8_t` is UB per C11 6.3.1.4. UBSan flagged the latter via expr_null/diff_i64_{and,or}_raw and expr_null/diff_f64_andor_chokes. Route AND/OR through two truthiness helpers that compare on the widened double and never cast it back to an integer: - truthy_intish(v, nullv) — false for 0 and for the operand's null sentinel. The fallback reads raw column memory, so a null arrives as the per-type sentinel widened to double (NULL_I16 / NULL_I32 / NULL_I64, with DATE/TIME stored as I32) rather than the NULL_I64 the VM kernel sees; `nullv` is derived per operand from the bound pointer type so I16/I32 nulls read as false, not just I64. - truthy_f64ish(v) — false for 0.0 and NaN (float null). Non-null truthiness is unchanged and null-input positions still agree with the VM kernel (documented "AND/OR with any null operand -> 0" and the fix_null_comparisons post-pass), keeping fallback ≡ fused. Add regression tests pinning fallback ≡ fused for nullable I64, I32 and I16 AND/OR operands (expr_null/diff_i{64,32,16}_{and,or}_raw). * fix(expr): preserve near-sentinel i64 truthiness --------- Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> Co-authored-by: Anton Kundenko <singaraiona@gmail.com> * ci: make Rayforce audit PR comments best-effort * ci: publish Rayforce audit comments from trusted workflow * ci: resolve fork PRs for audit commenter * perf: parallelize serial stages around group-by; unify binary-agg null semantics (#341) * wip(group): parallel binary aggregates (pearson/wavg/cov) via DA path Route binary co-moment aggregators through the dense-array (DA) group path instead of the hash scatter path. Adds sum_y/sumsq_y/sumxy co-moment slots to da_accum_t + per-row accumulation + per-worker merge; emit_agg_columns already finalises PEARSON/COV/WAVG/WSUM from the co-moments. Fixes poor multi-thread scaling of by-key binary aggregates (was ~2x, DA path scales ~9-12x like stddev). Verified vs numpy; diff comparator relaxed to 1e-9 combined abs+rel (1e-12 absolute tested bit-identical summation). Includes temporary RAY_GRPPROF phase instrumentation (to remove). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(group): merge binary-agg Sx as double for integer x-columns wavg/pearson accumulate Sx as double even when the x column is integer (e.g. wavg(bsize,bid), bsize=I32). The per-worker merge dispatched on the x-column type -> read the double bits as int64 -> garbage at >1 worker. Force float merge for binary aggs at all 3 sum-merge sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(group): merge binary-agg co-moments in parallel da_merge_fn path The parallel slot-range merge (da_merge_fn, taken when n_slots>=1024) merged sumsq but not the binary-aggregate co-moment arrays (sum_y/sumsq_y/sumxy). Multi-key pearson/cov/wavg over >=1024 dense slots produced wrong results at >1 worker. Add the DA_NEED_PAIR merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(group): remove temporary RAY_NO_V2/RAY_GRPPROF instrumentation The binary-agg DA fix lands on the default path (v2 declines CHAR-keyed binary group-bys -> legacy DA), so the debug env overrides are no longer needed. 3635/3635 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(if): parallel elementwise OP_IF fill; route trivial-branch if to eager exec_if always took the 'selected' lazy-branch path, whose scaffolding (true-count, id-list build, per-branch gather, scatter) is serial over ALL rows — every if-projection ran at single-core speed regardless of -c (100M numeric if: 2.2s at any core count). 1. exec_if_eager: one shared fixed-width elementwise fill, dispatched across the worker pool for len >= 64K (SYM sides warm their runtime-id LUT serially first — sym.c frozen-table rule, mirrors window.c). STR keeps the serial append path. 2. exec_if_selected: bail to eager when both branches are trivial (column scan / scalar const) and eager fills the type combination correctly — the lazy path only pays off when a branch is an expression worth restricting to its passing rows. Mixed numeric/string shapes stay on the selected path (its per-value string conversion). 100M rows local c24: numeric if 1883->310ms, sym if 2012->306ms. dazzle c48 canonical Q22: 2658->1333ms end-to-end. make test 3635/3635. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(filter): parallel bitmap->index build in exec_filter and sel_compact exec_filter ran two sequential 0..nrows sweeps (pass-count and match_idx build) before its parallel gather; sel_compact rebuilt match_idx from the rowsel serially. Both now use the classic 3-phase compaction: parallel per-chunk/per-seg counts, tiny serial prefix, parallel fill at disjoint offsets. Lazy/morsel-backed predicates keep the sequential sweep. 100M rows local c24: 2-col where-select 143->21.7ms (1.4x -> 6.6x scaling); if+where 1040->324ms. make test 3635/3635. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf: parallel where builtin, gather_by_idx, and chunk-task dispatch - ray_where_fn: 3-phase chunk compaction on the pool (was fully serial). - gather_by_idx: fixed-width value gathers dispatched over disjoint output ranges (null-bit propagation stays serial - shared-word bit writes would race). - exec_filter/where chunk phases now use ray_pool_dispatch_n (one task per chunk); ray_pool_dispatch morselizes total_elems by 1024, so passing chunk counts gave only ~2 tasks for 100M rows. 100M rows local c24: where 88->25ms, at-gather 80->31ms, 2-col where-select 138->20ms (6.8x). make test 3635/3635. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(review): harden parallel paths per skeptic review Blockers (DA binary-agg y-column): - eligibility now requires a plain numeric/temporal y; nullable integer/temporal y stays on the HT path (da_accum_row's pair branch has no y-side sentinel machinery - nulls would accumulate as values) - an FP y with HAS_NULLS sets da_any_nullable so nn[] is allocated and the emitter divides by the non-null PAIR count, not the group count Majors: - all new parallel gates require pool->n_workers > 0 (a -c 1 pool exists with 0 workers; ring fill + atomics + rc_sync were pure overhead, and the OP_IF eager reroute lost to the selected path serially - the Q22/Q25 c1 regression) - chunked dispatch_n call sites cap chunks at 1024 = the pool's initial ring capacity, so the ring never grows (dispatch_n clamps and silently DROPS tasks if ring growth fails -> uninitialized prefix entries -> OOB writes) - sel_compact seg fill switched to dispatch_n over seg-chunks (ray_pool_dispatch over segs gave 1 task under 8.4M rows) - gather_by_idx parallel path guarded by ray_parallel_flag == 0 (leaf utility, 35+ call sites; nested dispatch would corrupt the single-producer task ring) Nits: stray time.h include, restored v2-gate comment, RAY_PARALLEL_THRESHOLD symbol in pivot.c. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(group): pair-skip y-side nulls in the legacy HT binary-agg path Unify grouped binary-aggregate (pearson/cov/scov/wsum/wavg) null handling with the scalar reducers, the v2 engine and the DA path: a null on either side of the (x,y) pair now voids the whole pair on the legacy HT route too. - ght_compute_layout: a nullable y-side sets GHT_AF2_Y_NULLABLE and routes the layout to the null-aware accumulators. - accum_from_entry_nullable: pair-skip before nn++/sums. - Entry packing canonicalizes integer nulls so the accumulator can see them: NaN in F64-packed slots (a (double)sentinel cast previously read as a huge finite value — this also fixes nullable-int x beside an FP y), NULL_I64 in int-by-int slots. - Both HT emitters (radix + serial) divided pearson/cov/scov moments by the group row count instead of the accumulated pair count — wrong results whenever a group carried any null; now divide by nn. - The all-null-group guards wrote v=0.0 after ray_vec_set_null, so the common store overwrote the null sentinel — emit NULL_F64 instead. - DA eligibility now also rejects a y shorter than the scan (OP_CONST vector literal would read out of bounds). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZwH1gpAeLzDX2bxc4jz6s * test(agg): cross-path null coverage for grouped binary aggregates 46 assertions for wsum/wavg/pearson_corr/cov/scov over nullable inputs on all three grouped routes — v2 (plain-scan int key), DA (expression int key), legacy HT (expression key + nullable-int y; F64-packed and int-packed entry lanes) — against independently computed pair-skip truth, for all four x/y type combinations, plus an all-pairs-null group (wsum 0.0, typed nulls for the ratio/moment aggs) on every route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZwH1gpAeLzDX2bxc4jz6s * chore(review): shared dispatch-safety gate; single filter threshold - ops/internal.h ray_par_dispatch_ok(): workers + RAY_PARALLEL_THRESHOLD + ray_parallel_flag reentrancy check in one place; applied at exec_filter, sel_compact, exec_if_eager and the where builtin (local copy there — builtins.c cannot include ops/internal.h). - exec_filter: gate and table fallback derive from one row count (fidx_rows); note that pass_count from the parallel count phase is consumed by exec_filter_vec for vector inputs. - group.c: drop the never-read da_ctx_t.agg_pair_mask plumbing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZwH1gpAeLzDX2bxc4jz6s * chore(par): shared dispatch predicate, ring-cap constant, parallel-path test Follow-ups from the audit's non-blocking notes: - core/pool.h ray_pool_par_dispatch_ok(pool, n, min_elems): the single home for the dispatch-safety predicate (workers + element threshold + ray_parallel_flag reentrancy). The three hand-copies in ops/internal.h, lang/eval.c and ops/builtins.c are gone; all six gates call the shared one. - RAY_POOL_INIT_TASKS in core/pool.h replaces the hardcoded 1024 at the three dispatch_n chunk caps and in ray_pool_create, with a _Static_assert tying it to RAY_POOL_MAX_TASKS — lowering the initial ring capacity can no longer silently desync from the caps that rely on it. - test/rfl/query/parallel_paths_large.rfl: 200k-row coverage of every new pool-parallel branch (where, gather-by-index, exec_filter, sel_compact, OP_IF numeric and SYM fill incl. the serial LUT warm-up) against closed-form expected values. - RAY_F32 dropped from if_type_eager_ok's whitelist (if_fill_range has no F32 case; unreachable today, kept unreachable deliberately). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JZwH1gpAeLzDX2bxc4jz6s --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Anton Kundenko <singaraiona@gmail.com> * fix(aggr): preserve slice nullability in binary groups * fix(expr): avoid f64 null cast in fallback idiv integer output (#344) binary_range's OP_IDIV kernels for narrow integer output (I64/I32/I16/U8) computed `(intN_t)floor(lv/rv)` guarded only by `rv != 0.0`. That guard does not catch a NaN operand (`NaN != 0.0` is true), so a null float input yields `lv/rv == NaN`, `floor(NaN) == NaN`, and the subsequent cast to an integer type is undefined behavior — UBSan: "nan is outside the range of representable values of type 'long long'" at exec/expr_binary_f64_idiv_mod. Route the cast through the ray_cast_f64_to_{i64,i32,i16,u8}_null helpers, which map NaN to the canonical null sentinel (NULL_I64/I32/I16, 0 for the non-nullable U8) and saturate out-of-range finite results. The null post-pass (propagate_nulls_binary) already overwrites these positions, so final values are unchanged — this only removes the UB and yields the correct sentinel in-buffer. Mirrors the already-safe F64-output IDIV arm (ray_f64_fin) and the sibling casts fixed in "avoid f64 null casts to integers"; depends on those helpers. Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> * fix(group): avoid f64 null read cast in dense aggs (#343) Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> Co-authored-by: Anton Kundenko <singaraiona@gmail.com> --------- Co-authored-by: Karim <k.nassar@lynxtrading.com> Co-authored-by: Evgen <ebelozerov@lynxtrading.com> Co-authored-by: Evgen Byelozorov <belowzeroff@gmail.com> Co-authored-by: Evgen Belozerov <yevhenbielozorov@gmail.com> Co-authored-by: Serhii Savchuk <ser.vasilich@hotmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
try_load_link_sidecar(src/store/col.c) read a linked column's target tablesym name into a fixed 256-byte stack buffer (
freadof 255 bytes). A targetname longer than 255 bytes was silently truncated, so
ray_sym_interninterneda different symbol and the loaded column linked to the wrong table —
silent data corruption across a save/load round-trip. The writer side already
emits the full, untruncated name, so only the reader was capping.
(The
255limit insym.cbounds the number of dot-separated segments, notthe byte length of a name, so names longer than 255 bytes are legal and
reachable.)
The fix reads the whole sidecar into a buffer sized to the file, capped at 1 MiB
so a corrupt/oversized sidecar can't force a large allocation, using the
ray_alloc_raw/ray_free_rawpair already used elsewhere in the file. Thetrailing-whitespace trim and best-effort semantics (any failure leaves the
column unlinked) are preserved.
Test
link/persistence_long_target_namelinks a column through a 300-byte targetname, saves, loads, and asserts the loaded
link_targetstill resolves to theoriginal symbol. Verified as a genuine regression test — with the old reader the
suite reports:
and with the fix it passes.
Full suite under the debug ASan + UBSan build:
3632 of 3633 passed (1 skipped, 0 failed).Checklist
dev(notmaster)fix:)makebuilds cleanly (no new warnings)make testpasses; tests added/updated for behaviour changes